Loans: Runtime API for updating the portfolio with fake prices #1748
Conversation
lemunozm
added
Q3-medium
|
Blocked by #1739 |
| PriceCollectionInput::Empty => BTreeMap::default(), | ||
| PriceCollectionInput::Custom(prices) => prices.into(), | ||
| PriceCollectionInput::FromRegistry => Self::registered_prices(pool_id)?, |
There was a problem hiding this comment.
What is the benfit of using the PriceCollectionInput here vs just using a BTreeMap as the parameter?
There was a problem hiding this comment.
There was a problem hiding this comment.
Empty is just sugar for Custom([]) and could be removed. But the caller should be able to choose between Custom and FromRegistry.
There was a problem hiding this comment.
I prefer @lemunozm's proposal over just using a BTreeMap as it is more explicit which is always helpful for runtime API which can be expected to be used by externals as well.
There was a problem hiding this comment.
Okay, you changed the valuation parameters of the runtime-api. Sounds good!
The most important thing though would be to mimic closing an epoch with
- faked prices
- Optional
Returning:
- Fullfillment amounts for each tranche for both invest and redeem
- New pool-reserve
- New Tranche prices
There was a problem hiding this comment.
Ok, I guess somehow this should be amalgamated with some other pallet-pool-system calls to get this, right?
Then I propose to create another in the PoolSystem RuntimeAPI, maybe called simulate_close_epoch or similar, where all these calls are performed to return all the required values above.
There was a problem hiding this comment.
Great, I'll do in another PR pool-related.
| PriceCollectionInput::Empty => BTreeMap::default(), | ||
| PriceCollectionInput::Custom(prices) => prices.into(), | ||
| PriceCollectionInput::FromRegistry => Self::registered_prices(pool_id)?, |
There was a problem hiding this comment.
I prefer @lemunozm's proposal over just using a BTreeMap as it is more explicit which is always helpful for runtime API which can be expected to be used by externals as well.
| @@ -1208,7 +1236,8 @@ pub mod pallet { | |||
| } | |||
|
|
|||
| fn update_nav(pool_id: T::PoolId) -> Result<T::Balance, DispatchError> { | |||
| Ok(Self::update_portfolio_valuation_for_pool(pool_id)?.0) | |||
| Self::update_portfolio_valuation_for_pool(pool_id, PriceCollectionInput::FromRegistry) | |||
There was a problem hiding this comment.
A quick comprehension question: Our rather newer NAV calculation runtime API calls Loans::update_nav(pool_id) such that the Pool Fees NAV calculation can be based on that. Say the loans NAV is updated with a price collection input in block i and in the next block i+1 we are querying the NAV via PoolsApi::nav(pool_id) which performs Loans::update_nav(pool_id) with an update based on PriceCollectionInput::FromRegistry. IIUC, the prices from the update in block i are still cached. What I want to find out is if Loans::update_nav can make the Loans NAV less accurate if it was previously based on a custom price list.
There was a problem hiding this comment.
It was previously based on the price registry, so both lines above are exactly identical. From a pool fees perspective, nothing has changed. The only open door (right now) is for the RuntimeAPI.
mustermeiszer
added
the
D5-breaks-api
There was a problem hiding this comment.
One minor nit change I would like to see: Runtime API can be versioned via #[api_version($VERSION)] as part of the decl_runtime_apis! macro (example). However, we have not made use of this feature so far. Of course, we should to make the breaking changes explicit.
|
Good point. Just one question: should we bump up the version even if the upgrade is retro-compatible? (it means just an addition) |
|
@mustermeiszer can we merge #1739 before? |
lemunozm
dismissed
mustermeiszer’s stale review
The base branch was changed.
lemunozm
force-pushed
the
loans/fake-prices-api
branch
from
b79d5a2 to
12a3beb
Compare
Definitely not a blocker because it's not breaking. In the future, I would still like to make real use of versioning because it can be tied to the earliest supported spec version for that API. For instance, SubQuery recently had issues with the new PoolsNAV API because it was attempted at blocks which didn't support it. But in order for this to fully work, Apps needs to add spec version granularity to the type and RtApi decorations. |
|
Good reasoning, you've convince me! => 24adcea |
Description
Add a Runtime API to emulate a portfolio update with custom prices.